Skip to content

Fix charge reading from PDBQTParser#4283

Merged
orbeckst merged 3 commits into
MDAnalysis:developfrom
pgbarletta:fix_pdbqt_parser
Oct 15, 2023
Merged

Fix charge reading from PDBQTParser#4283
orbeckst merged 3 commits into
MDAnalysis:developfrom
pgbarletta:fix_pdbqt_parser

Conversation

@pgbarletta

@pgbarletta pgbarletta commented Sep 6, 2023 •
edited by orbeckst
Loading

Copy link
Copy Markdown
Contributor

Fixes #4282

Changes made in this Pull Request:

  • Columns 67 to 70 from PDBQT files are ignored instead of trying to parse them as atom charges.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin

📚 Documentation preview 📚: https://mdanalysis--4283.org.readthedocs.build/en/4283/

@github-actions

github-actions Bot commented Sep 6, 2023 •
edited
Loading

Copy link
Copy Markdown

Linter Bot Results:

Hi @pgbarletta! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/6525284582/job/17717806569

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@codecov

codecov Bot commented Sep 6, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0b9bfd5) 93.40% compared to head (f015129) 93.37%.
Report is 11 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4283      +/-   ##
===========================================
- Coverage    93.40%   93.37%   -0.04%     
===========================================
  Files          170      184      +14     
  Lines        22256    23403    +1147     
  Branches      4071     4075       +4     
===========================================
+ Hits         20788    21852    +1064     
- Misses         951     1035      +84     
+ Partials       517      516       -1
Files Coverage Δ
package/MDAnalysis/coordinates/PDBQT.py 83.50% <ø> (ø)
package/MDAnalysis/topology/PDBQTParser.py 100.00% <100.00%> (ø)

... and 18 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

richardjgowers
richardjgowers approved these changes Sep 17, 2023
View reviewed changes

@richardjgowers richardjgowers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @pgbarletta this looks like a good catch. If you have time adding a test (or updating whatever pdbqt file we currently have) to include a remark in cols 67-70 would make sure this behaviour is tested

orbeckst
orbeckst requested changes Sep 17, 2023
View reviewed changes

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that fails without the fix and passes with it.

Please also include a link to the format document in the docs. The more we can say very clearly which format we are implementing the better.

Finally, add a versionchanged (take into account the "footnote" field in coulmns ....) because it's possible that the standard changed without us knowing and then we want to have a record of when we changed.

@pep8speaks

pep8speaks commented Sep 20, 2023 •
edited
Loading

Copy link
Copy Markdown

Hello @pgbarletta! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 91:80: E501 line too long (80 > 79 characters)

Line 109:22: W291 trailing whitespace

Comment last updated at 2023-10-15 16:43:30 UTC

@pgbarletta pgbarletta requested a review from orbeckst September 21, 2023 00:40
orbeckst
orbeckst approved these changes Oct 15, 2023
View reviewed changes

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your changes. All looking good (will just make a minor reformatting change).

I am very sorry, the review slipped through. As always, please feel free to ping me if there's no action in a few days.

Comment thread package/MDAnalysis/topology/PDBQTParser.py Outdated
@orbeckst orbeckst self-assigned this Oct 15, 2023
@orbeckst orbeckst merged commit 913ca7b into MDAnalysis:develop Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@orbeckst orbeckst orbeckst approved these changes
@richardjgowers richardjgowers richardjgowers approved these changes

Assignees

@orbeckst orbeckst

Labels

Component-Readers Component-Topology Format-PDBQT maintainability

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PDBQTParser.parse() assigns wrong field to charges

5 participants

@pgbarletta @pep8speaks @orbeckst @richardjgowers @IAlibay